fix(Web): Dark Mode isn't friendly to find#7721
Conversation
|
@cocojojo5213 is attempting to deploy a commit to the Flagsmith Team on Vercel. A member of the Team first needs to authorize it. |
github-actions
Bot
added
the
front-end
|
Validation note: pre-commit.ci passes and I ran the focused frontend checks listed in the PR description locally. The current Vercel failures say "Authorization required to deploy", and the GitHub Actions frontend workflow is waiting for maintainer approval for this fork PR. |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces system theme preference support ('light', 'dark', 'system') alongside the legacy dark mode setting, adding a new theme selection dropdown and updating the dark mode utility and navigation components accordingly. Feedback highlights two key issues: a race condition in account-store.js where AsyncStorage.clear() could clear restored theme preferences due to its asynchronous nature, and a positioning issue in ThemeModeDropdown where the dropdown can become misaligned during page scroll or window resize.
| AsyncStorage.clear() | ||
| if (darkMode) { | ||
| storageSet('dark_mode', darkMode) | ||
| } | ||
| if (themePreference) { | ||
| storageSet('theme_preference', themePreference) | ||
| } |
There was a problem hiding this comment.
Since AsyncStorage.clear() is an asynchronous operation that returns a Promise, executing synchronous storageSet calls immediately after it creates a race condition. The storage might be cleared after the theme preferences are restored, causing them to be lost on logout. Wrapping the restoration in the .then() callback of AsyncStorage.clear() ensures they are safely preserved.
AsyncStorage.clear().then(() => {
if (darkMode) {
storageSet('dark_mode', darkMode)
}
if (themePreference) {
storageSet('theme_preference', themePreference)
}
})| useLayoutEffect(() => { | ||
| if (!isOpen || !dropDownRef.current || !btnRef.current) return | ||
| const listPosition = calculateListPosition( | ||
| btnRef.current, | ||
| dropDownRef.current, | ||
| ) | ||
| dropDownRef.current.style.top = `${listPosition.top}px` | ||
| dropDownRef.current.style.left = `${listPosition.left}px` | ||
| }, [isOpen]) |
There was a problem hiding this comment.
The dropdown position is currently calculated only once when isOpen changes. Since the dropdown is rendered via a portal to document.body, scrolling the page or resizing the window while the dropdown is open will cause it to detach and become misaligned from the trigger button. Adding event listeners for resize and scroll (using capture phase to catch scrolling in any container) ensures the dropdown remains correctly positioned.
useLayoutEffect(() => {
if (!isOpen || !dropDownRef.current || !btnRef.current) return
const updatePosition = () => {
if (!btnRef.current || !dropDownRef.current) return
const listPosition = calculateListPosition(
btnRef.current,
dropDownRef.current,
)
dropDownRef.current.style.top = `${listPosition.top}px`
dropDownRef.current.style.left = `${listPosition.left}px`
}
updatePosition()
window.addEventListener('resize', updatePosition)
window.addEventListener('scroll', updatePosition, true)
return () => {
window.removeEventListener('resize', updatePosition)
window.removeEventListener('scroll', updatePosition, true)
}
}, [isOpen])
3 participants
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Closes #7702
Adds an always-available theme selector to the app header so users can switch theme without leaving their current page. The selector is shown in the existing desktop top nav and as a compact mobile header action.
dark_modevalue for compatibility.Review effort: 3/5
How did you test this code?
npx eslint --fix web/project/darkMode.ts web/project/darkMode.test.ts web/components/DarkModeSwitch.tsx web/components/navigation/Nav.tsx web/components/navigation/navbars/TopNavbar.tsx common/stores/account-store.jsnpm run test:unit -- --runTestsByPath web/project/darkMode.test.tsnpm run bundlegit diff --checkI also checked the touched files against
tsc; the full repository typecheck currently reports existing errors outside this change.